Skip to content

feat(vault): Added 1Password secrets provider - #5154

Open
bjerringgaard wants to merge 6 commits into
Dokploy:canaryfrom
bjerringgaard:canary
Open

bjerringgaard wants to merge 6 commits into
Dokploy:canaryfrom
bjerringgaard:canary

Conversation

@bjerringgaard

@bjerringgaard bjerringgaard commented Aug 21, 2026 •

Copy link
Copy Markdown

What is this PR about?

Added 1Password as an option for the vault-providers.
Making use of 1password's newer "Environments" feature.
Relying on 1Password's Service Account Authentication

Checklist

Before submitting this PR, please make sure that:

  • [✔︎ ] You created a dedicated branch based on the canary branch.
  • [✔︎ ] You have read the suggestions in the CONTRIBUTING.md file https://github.com/Dokploy/dokploy/blob/canary/CONTRIBUTING.md#pull-request
  • [✔︎] You have tested this PR in your local instance. If you have not tested it yet, please do so before submitting. This helps avoid wasting maintainers' time reviewing code that has not been verified by you.

Issues related (if applicable)

closes #1777
closes #1192

Greptile Summary

The PR adds 1Password Environments as a vault provider using service-account authentication.

  • Adds the provider schema, persistence migration, server-side client, and vault dispatch integration.
  • Adds dashboard configuration fields, provider labeling, and icon support.
  • Adds tests for authentication, secret retrieval, connection checks, name listing, and reference resolution.
  • The previously reported unbounded request path is now protected by the shared vault timeout helper.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; the previously unbounded 1Password SDK operations now reject through the shared vault timeout wrapper.

Reviews (4): Last reviewed commit: "fix: added tests" | Re-trigger Greptile

Context used:

@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 21, 2026
Comment thread packages/server/src/utils/vault/onepassword.ts Outdated

@narcisonunez narcisonunez left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests for this. You can add them in __test__/env/vault.test.ts with a describe as the other providers do.

@bjerringgaard

Copy link
Copy Markdown
Author

@narcisonunez Tests have now been added.

@bjerringgaard bjerringgaard changed the title feat(vault): Added 1Password as Secrets provider feat(vault): Added 1Password secrets provider Aug 25, 2026
@mitc-gjuge

Copy link
Copy Markdown
Contributor

I tested this branch against a real 1Password account on a local instance, since it looked like nobody had yet. The implementation itself works well, but there is a blocking issue that only shows up at runtime.

Blocking: the enum migration is gone, so creating a provider fails

The branch adds onepassword to the VaultProviderType enum in packages/server/src/db/schema/vault-provider.ts, but no migration adds that value to the Postgres enum. The branch's _journal.json ends at 0185, and no .sql file mentions onepassword.

The original commit f79a824 did include it — apps/dokploy/drizzle/0186_awesome_legion.sql:

ALTER TYPE "public"."VaultProviderType" ADD VALUE 'onepassword';

Commit 811e76c ("Remove unused journal entry for tag 0186_awesome_legion") then deleted that file, its snapshot and the journal entry. The 0186 number was indeed already taken on canary (0186_tearful_dragon_man), but the migration was removed without being regenerated at a free number.

Reproduced on a fresh instance built from this branch:

$ psql -c "select enumlabel from pg_enum e join pg_type t on t.oid=e.enumtypid
           where t.typname='VaultProviderType'"
hashicorp, infisical, aws, doppler, azure, scaleway      <- no onepassword

Creating a 1Password provider then fails with INTERNAL_SERVER_ERROR, and the underlying cause is:

ERROR:  invalid input value for enum "VaultProviderType": "onepassword"

I confirmed the causality: applying just ALTER TYPE "public"."VaultProviderType" ADD VALUE 'onepassword'; by hand makes provider creation succeed immediately, and everything below then works.

This is invisible in review — the TypeScript is perfectly consistent — and only surfaces the first time someone actually creates a 1Password provider. The fix is to rebase on canary and re-run pnpm --filter=dokploy run migration:generate, which will emit the same statement at the next free number (0190 at the time of writing). Happy to send that as a PR against your fork if it helps.

Everything else works against a real account

With the enum value present, tested end to end against a real 1Password Environment and service account:

  • testConnection succeeds against the live API.
  • listSecretNames returns the Environment's variables (['DB_PASSWORD', 'ADMIN_PASSWORD', 'USER_TOKEN']), so bulk import and env autocomplete work.
  • resolveVaultReferences resolves a single ref, and several refs mixed with plain lines, to the correct values.
  • The token is masked as ******** in API responses, and re-saving the provider with the mask correctly keeps the stored token.
  • Error paths are clear and specific:
    • missing variable → 1Password: variable "PAS_LA" not found in environment "4dtl..."
    • bad environment ID → 1Password: failed to read environment "id_invalide_xyz" (invalid user input: ...)
    • unassigned provider and unknown provider both produce the expected resolveVaultReferences errors.

pnpm -r run typecheck passes and the vault suite is green (46 tests).

Both providerLabels maps are updated, including the one in show-vault-providers.tsx that is easy to miss, and SENSITIVE_FIELDS is correct.

Two minor observations, not blockers

testConnection passes on an empty Environment. My Environment had no variables at first and the test still reported success, since it only checks that getVariables doesn't throw. Defensible, but a green "Test Connection" doesn't tell the user they'll actually see anything. A hint when the variable list comes back empty might save some confusion.

vaultFetchWithTimeout bounds the caller, not the work. The Promise.race correctly stops the caller from hanging, which was the original concern, but the underlying SDK call keeps running since there's no AbortSignal to pass through. Worth a comment so nobody later assumes it cancels the request.

One note on scope

Both open requests for this feature ask for the other 1Password model. #1192 asks specifically for op://development/aws/Access Keys/access_key_id, and #1777 for access to "credentials in user-authorized vaults" — that's client.secrets.resolve() over op://vault/item/field, whereas this PR reads 1Password Environments via getVariables(). Environments also carry real setup friction: an Owner has to turn the policy on, programmatic read access is in public beta, and because Environment access on a service account is immutable, an existing service account can't be granted access to a newly created Environment — you have to create a new one through the web wizard, as the CLI can't do it.

That's not a criticism of this PR, which does what it says. I'd be glad to send a follow-up adding op:// reference support on top of it once this lands, so both models are covered by the same provider. Just wanted to flag it here rather than after the fact.

@bjerringgaard
bjerringgaard force-pushed the canary branch 2 times, most recently from 3040236 to b0c7790 Compare September 2, 2026 06:35
@bjerringgaard

Copy link
Copy Markdown
Author

@mitc-gjuge
Seems the missing enum was caused in the merges to upstream, i have added it back in.

On Minor observations
testConnection
Test connection is in my opinion not wrong, as we are testing if the service provider credentials has access to its allows environment, which could be updated / filled in later, I do see your point, a hint might be useful, but i wouldn't say necessary.

vaultFetchWithTimeout
As im using the SDK instead of the established pattern for API usage, the abort controller is not available.
What would be your suggestion for handling this correctly?

On the scope note
Im aware of the op:// and it would make sense to add in, i chose for this PR to run with the actual environment setup that 1pass has now added (yes it is under developer settings) in my opinion it is the option that 1pass is pushing as a secrets provider, keeping it inline with the other providers currently present.
It also allows for more closely controlling access to what environments the provider has access to. but adding in the original op:// might be a good call for later, i was more concerned with implementing the way 1pass is pushing towards.

@mitc-gjuge

Copy link
Copy Markdown
Contributor

Thanks for picking these up so quickly — I re-tested the migration fix this morning and it lands correctly. On a database rebuilt from your branch the enum comes out as hashicorp, infisical, aws, doppler, azure, scaleway, phase, onepassword, and creating a 1Password provider now succeeds where it failed before. Worth noting you also avoided a second trap I hit on my own PRs today: your 0191_luxuriant_sunspot has when=1788330424769, later than canary's 0190_nappy_anita_blake (1788300390648). Drizzle applies migrations by comparing timestamps against the last applied one, so an older when would have been silently skipped even with a free number.

testConnection — you're right, and I'd drop it. Testing that the credentials can reach the environment is the correct contract; whether that environment currently holds variables is the user's business, and an empty one is a legitimate state. My own confusion came from having an empty environment on the first run, which is an argument for a hint at most, not for changing what the call verifies.

vaultFetchWithTimeout — I looked for a better answer and I don't think there is one. In @1password/sdk@0.5.0, getVariables(environmentId: string) takes no options object, ClientConfiguration exposes only auth, integrationName, integrationVersion, oidcFetcher and workloadDetails, and there is no occurrence of AbortSignal, AbortController or cancel anywhere in the shipped typings. There is simply nothing to pass a cancellation into.

So Promise.race is the right call here — it bounds the caller, which was the actual concern, and that's all any wrapper can do from outside. The only thing I'd add is a line of comment on the helper saying so explicitly: that it stops the caller from hanging but does not cancel the underlying request, which keeps running to completion. Not a code change — just so nobody later reads "timeout" and assumes the work was abandoned. If the SDK ever grows a signal parameter, that comment is also where you'd notice it should be revisited.

On scope — your reasoning holds, and I'd not want it bolted onto this PR. Environments are what 1Password is steering people towards, the per-environment access boundary is a real advantage over a broad service account, and shipping the narrower thing first is the right shape for review.

The one thing I'd flag is that the two open requests ask for the other model: #1192 spells out AWS_ACCESS_KEY_ID="op://development/aws/Access Keys/access_key_id" literally, and #1777 asks for access to credentials in user-authorized vaults. Neither is served by getVariables. There's also some friction users will hit — an Owner has to enable the Environments policy, programmatic read access is still in public beta, and because Environment access on a service account is immutable, an existing service account can't be granted access to a newly created Environment at all; you have to make a new one through the web wizard, since the CLI can't do it. I went through exactly that yesterday to test this branch.

None of that argues against merging this. It argues for a follow-up, which I'm happy to write once this lands: same onepassword provider type, environmentId becomes optional, and a ref containing / resolves through client.secrets.resolve("op://" + ref) while a bare name keeps going to getVariables. That leaves your behaviour untouched and covers both models under one provider. Your call whether you'd rather own that yourself — it's your integration, I'd just as happily review it.

@mitc-gjuge

Copy link
Copy Markdown
Contributor

Heads-up: merging #5257 put this PR in conflict — canary now carries 0191_cool_christian_walker, which collides with your 0191_luxuriant_sunspot. Sorry for the churn, that one was mine.

Worth flagging one subtlety before you rebase. Renaming the file to 0192 would clear the conflict and turn CI green, but it wouldn't actually work. Drizzle decides what to apply by comparing when against the single most recent applied migration:

if (!lastDbMigration || Number(lastDbMigration.created_at) < migration.folderMillis)

and yours is now earlier than canary's:

migration when
canary 0191_cool_christian_walker 1788332224024
this PR 0191_luxuriant_sunspot 1788330424769 — ~30 min earlier

So on any database that has already run canary's 0191, yours would be silently skipped, the enum would never get onepassword, and it would only surface later as invalid input value for enum "VaultProviderType" the first time someone creates a 1Password provider.

Regenerating after the rebase — pnpm --filter=dokploy run migration:generate — gives it a current timestamp and avoids that. I hit exactly this on #5258 an hour ago and regenerated as 0192 rather than renaming.

@mitc-gjuge

Copy link
Copy Markdown
Contributor

@narcisonunez — small heads-up on #5154, if you have a moment. Both workflows on
292e0617a are stuck at action_required, so its checks aren't red, they've
never been allowed to start; a one-click approval would show where it stands.

Your 24 August review asked for tests in __test__/env/vault.test.ts — they
landed on the 25th, and the PR is mergeable again after this morning's rebase.

@appscaptain

Copy link
Copy Markdown

Amazing PR. Can't wait! Will this coming version also enable OP password references so the environments are not necessary?

@mitc-gjuge

Copy link
Copy Markdown
Contributor

Not in this one — it reads 1Password Environments only, so an Environment ID is
required and the secrets have to live there.

op:// references are a different API, and they're the natural follow-up once
this PR lands.

@bjerringgaard

Copy link
Copy Markdown
Author

@appscaptain The first version of this PR, is about adding the env feature 1password themselves are pushing for.
using the newer environment feature, this also allows for more fine grained control of access, useful for more non hobby users of dokploy.

I plan on creating a follow up PR, adding the "op://" handler more or less right after this PR gets merged.

@bjerringgaard

Copy link
Copy Markdown
Author

@narcisonunez Would it be possible to have the workflows run on this PR once again?
Your "request changes" where made in regards to adding the final testes, these have been added about a month ago.

@appscaptain

Copy link
Copy Markdown

@mitc-gjuge @bjerringgaard

Sounds good with the plan. Looking very much forward to it. The reason why I am asking for the op support is that:
A. Many project use the same credentials (ex. multiple Stripe keys for live, sandboxes etc.) and with Environments they have to be duplicated across multiple environments because 1P does not support op:// references in the environment feature.
B. I also need the same keys outside environments for other things, so again I need to duplicate them into the environment.
C. When you need to add a new project you can't add the environments allocated to the existing service key token because 1P doesn't allow this edit.
D. To create a new environment for new projects you need to go through a service account setup for each of them which is a manual cumbersome process on the website.
E. And if you want to keep multiple environments under the same service accounts token then, since you have to recreate it, you then have to go update the service token to the new values everywhere you used that environments.
F. If you need any automation through 1password cli or the MCP you can't automate the service key creation either.

So the process of creation and maintenance of environment data becomes a mess I think

So while I like 1Password idea of the environments, due to its implementation with all these limitations, it is just way more practical to simply use OP:// references as this overcomes every single one of those issues.

@sbsrnt

sbsrnt commented Oct 6, 2026

Copy link
Copy Markdown

@narcisonunez @Siumauricio any updates?

@alxwlw

alxwlw commented Oct 8, 2026

Copy link
Copy Markdown

We'd use this in production too — all our secrets already live in 1Password, so this PR (and the op:// follow-up) would let us drop a separate secret pipeline for Dokploy. The requested tests landed on Aug 25 and it was verified against a real account; @narcisonunez could you re-run the workflows / re-review when you get a chance?

@appscaptain

Copy link
Copy Markdown

Yeah, also really need this. As I couldn't wait I merged this into my own Dokploy install across 3 servers a few weeks ago and it has been working perfectly with the 1Password environments - no issues whatsoever. But really missing the op:// support though, as that would be way easier to maintain than Environments.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate with 1Password 1Password integration

6 participants